fix(browser): Use `apply` rather than `call` in `try-catch` integration #4695

lobsterkatie · 2022-03-08T16:47:42Z

Users have reported running into a bug in Chrome wherein calling addEventListener or requestAnimationFrame too many times on window eventually throws an error when our try-catch integration is running, specifically because of how we wrap those functions. In the discussion of that bug, one user reported that replacing our call call with an apply call in our wrapping functions solved the problem for him.

This makes that change, in the hopes it will fix the problem for everyone.

Fixes #3388
Fixes #2074

Ref: https://getsentry.atlassian.net/browse/WEB-669

AbhiPrasad

Now that we're here, can we add a integration test for this?

github-actions · 2022-03-08T16:56:37Z

size-limit report

Path	Base Size (4e65cb7)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.79 KB	19.79 KB	-0.01% 🔽
@sentry/browser - ES5 CDN Bundle (minified)	63.38 KB	63.38 KB	+0.01% 🔺
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.44 KB	18.44 KB	+0.01% 🔺
@sentry/browser - ES6 CDN Bundle (minified)	56.41 KB	56.41 KB	+0.02% 🔺
@sentry/browser - Webpack (gzipped + minified)	22.27 KB	22.27 KB	+0.01% 🔺
@sentry/browser - Webpack (minified)	76.34 KB	76.35 KB	+0.01% 🔺
@sentry/react - Webpack (gzipped + minified)	22.3 KB	22.3 KB	+0.01% 🔺
@sentry/nextjs Client - Webpack (gzipped + minified)	46.43 KB	46.43 KB	+0.01% 🔺
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.69 KB	26.69 KB	+0.01% 🔺
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.01 KB	25.02 KB	+0.01% 🔺

kamilogorek · 2022-03-08T18:25:26Z

cc @BYK

lobsterkatie · 2022-03-08T18:29:03Z

Now that we're here, can we add a integration test for this?

What test would you add, exactly? As far as I know, this is a workaround for a bug that only exists in Chrome 74 and 75, last current in mid-2019. Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

kamilogorek

LGTM. Agree that tests would be nice addition.

BYK · 2022-03-08T18:40:39Z

I am not sure if this will actually fix the issue. My guess is the this reference on that .call() call being undefined or null and Chrome complaining for some reason?

AbhiPrasad · 2022-03-08T18:40:48Z

Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

This, but we can skip for now if you want

lobsterkatie · 2022-03-08T18:54:00Z

I am not sure if this will actually fix the issue. My guess is the this reference on that .call() call being undefined or null and Chrome complaining for some reason?

To be honest, I'm not sure, either. I can confirm that using the repro provided in the older issue, this error shows up in Chrome 74 and 75, but a) not in Chrome 76, and b) not if you use apply rather than call. That said, this may be a totally separate problem which just happens to manifest the same way.

Before digging further, though, I'd like to try this, because it's a painless fix. If it doesn’t work, we can debug further. (If that does happen, because it turns out this is in fact a different problem, it'd be great if someone could figure out a way to reproduce the new problem.)

This fix should land in 6.18.3, which will be released before the end of the week.

lobsterkatie · 2022-03-08T18:56:53Z

Or do you just mean integration tests for addEventListener and requestAnimationFrame in general?

This, but we can skip for now if you want.

Let's get this out so folks can try it, and handle the tests separately. Also, FWIW, we do actually have integration tests for both of those, they're just not playwright tests:

sentry-javascript/packages/browser/test/integration/suites/builtins.js

Lines 2 to 24 in 008982f

    
           it("should capture exceptions from event listeners", function() { 
        
             return runInSandbox(sandbox, function() { 
        
               var div = document.createElement("div"); 
        
               document.body.appendChild(div); 
        
               div.addEventListener( 
        
                 "click", 
        
                 function() { 
        
                   window.element = div; 
        
                   window.context = this; 
        
                   foo(); 
        
                 }, 
        
                 false 
        
               ); 
        
               var click = new MouseEvent("click"); 
        
               div.dispatchEvent(click); 
        
             }).then(function(summary) { 
        
               // Make sure we preserve the correct context 
        
               assert.equal(summary.window.element, summary.window.context); 
        
               delete summary.window.element; 
        
               delete summary.window.context; 
        
               assert.match(summary.events[0].exception.values[0].value, /baz/); 
        
             }); 
        
           });

and

sentry-javascript/packages/browser/test/integration/suites/builtins.js

Lines 87 to 99 in 008982f

    
           it("should capture exceptions inside callback", function() { 
        
             // needs to be visible or requestAnimationFrame won't ever fire 
        
             sandbox.style.display = "block"; 
        
             return runInSandbox(sandbox, { manual: true }, function() { 
        
               requestAnimationFrame(function() { 
        
                 window.finalizeManualTest(); 
        
                 foo(); 
        
               }); 
        
             }).then(function(summary) { 
        
               assert.match(summary.events[0].exception.values[0].value, /baz/); 
        
             }); 
        
           });

.

BYK · 2022-03-08T19:01:15Z

@lobsterkatie .apply() is significantly slower than .call() so this may cause you some headaches, especially with time-sensitive things like requestAnimationFrame().

EDIT: Seems like the slowness does not exist on Chromium-based browsers. Firefox showed a significant dip in .apply() here: https://www.measurethat.net/Benchmarks/Show/398/0/direct-call-vs-bind-vs-call-vs-apply (but MS Edge only showed a marginal drop)

lobsterkatie · 2022-03-08T19:35:05Z

@lobsterkatie .apply() is significantly slower than .call() so this may cause you some headaches, especially with time-sensitive things like requestAnimationFrame().

Interesting...

I hadn't heard that before, so I just went and did a little research. Most of what I found was from the early aughts, but I did find one site which will benchmark it directly, and will also let you see other users' results, so you get to see a wider variety of platforms, browsers, and versions. The upshot is that you're right when it comes to Firefox (and the difference is pretty drastic), but in Safari and Chrome they're nearly identical.

Firefox:

Safari:

Chrome:

That said, I am an OG FF lover and mean no offense to its userbase, but it is also down to ~4% market share. So I'd like to go ahead and see if this fixes the problem, and if it doesn't, we can always revert and try a different strategy.

use apply rather than call in try-catch integration

9dd5c65

AbhiPrasad reviewed Mar 8, 2022

View reviewed changes

kamilogorek self-requested a review March 8, 2022 18:26

kamilogorek approved these changes Mar 8, 2022

View reviewed changes

lobsterkatie merged commit 3784ee1 into master Mar 8, 2022

lobsterkatie deleted the kmclb-try-catch-use-apply branch March 8, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(browser): Use `apply` rather than `call` in `try-catch` integration #4695

fix(browser): Use `apply` rather than `call` in `try-catch` integration #4695

lobsterkatie commented Mar 8, 2022 •

edited

Loading

AbhiPrasad left a comment

github-actions bot commented Mar 8, 2022

kamilogorek commented Mar 8, 2022

lobsterkatie commented Mar 8, 2022 •

edited

Loading

kamilogorek left a comment

BYK commented Mar 8, 2022

AbhiPrasad commented Mar 8, 2022 •

edited

Loading

lobsterkatie commented Mar 8, 2022

lobsterkatie commented Mar 8, 2022 •

edited

Loading

BYK commented Mar 8, 2022 •

edited

Loading

lobsterkatie commented Mar 8, 2022

fix(browser): Use apply rather than call in try-catch integration #4695

fix(browser): Use apply rather than call in try-catch integration #4695

Conversation

lobsterkatie commented Mar 8, 2022 • edited Loading

AbhiPrasad left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2022

size-limit report

kamilogorek commented Mar 8, 2022

lobsterkatie commented Mar 8, 2022 • edited Loading

kamilogorek left a comment

Choose a reason for hiding this comment

BYK commented Mar 8, 2022

AbhiPrasad commented Mar 8, 2022 • edited Loading

lobsterkatie commented Mar 8, 2022

lobsterkatie commented Mar 8, 2022 • edited Loading

BYK commented Mar 8, 2022 • edited Loading

lobsterkatie commented Mar 8, 2022

fix(browser): Use `apply` rather than `call` in `try-catch` integration #4695

fix(browser): Use `apply` rather than `call` in `try-catch` integration #4695

lobsterkatie commented Mar 8, 2022 •

edited

Loading

lobsterkatie commented Mar 8, 2022 •

edited

Loading

AbhiPrasad commented Mar 8, 2022 •

edited

Loading

lobsterkatie commented Mar 8, 2022 •

edited

Loading

BYK commented Mar 8, 2022 •

edited

Loading